Add per-test fieldcompare tolerance and skip_compare#847
Add per-test fieldcompare tolerance and skip_compare#847PranjalManhgaye wants to merge 3 commits into
Conversation
Allow overriding fieldcompare rtol via tests.yaml and optionally skip the compare step. Re-enable previously quarantined tests with tolerance 1e-2 for near-zero exported fields.
Wrap long tolerance/skip_compare lines so check_style pre-commit passes.
MakisH
changed the title
MakisH
left a comment
MakisH
left a comment
There was a problem hiding this comment.
Thanks for implementing!
Here are some comments to apply before testing on GHA.
|
|
||
| # Excluded: | ||
| # *elastic-tube-3d_fluid-openfoam_solid-fenics # too small values to compare | ||
| - *elastic-tube-3d_fluid-openfoam_solid-fenics |
There was a problem hiding this comment.
Lists in this file are meant to be sorted alphabetically. Similar issue in the other previously commented-out tests in this file.
| -rtol 3e-7 | ||
| ``` | ||
|
|
||
| The default relative tolerance is `3e-7`. Per-test overrides are available in `tests.yaml` via `tolerance` (passed to fieldcompare as `-rtol`). Set `skip_compare: true` to skip the comparison step and only verify that build and run succeed. |
There was a problem hiding this comment.
| The default relative tolerance is `3e-7`. Per-test overrides are available in `tests.yaml` via `tolerance` (passed to fieldcompare as `-rtol`). Set `skip_compare: true` to skip the comparison step and only verify that build and run succeed. | |
| The default relative tolerance (`-rtol`) is `3e-7`. Per-test overrides are possible in `tests.yaml` (e.g., `tolerance: 1e-2`). Set `skip_compare: true` to skip the comparison step and only verify that the build and run steps succeed. |
There was a problem hiding this comment.
Why does the generate_reference_results.py need information about numerical regression checks?
| @@ -0,0 +1 @@ | |||
| - Add optional `tolerance` and `skip_compare` fields to `tests.yaml` for per-test fieldcompare configuration closes #837. | |||
There was a problem hiding this comment.
| - Add optional `tolerance` and `skip_compare` fields to `tests.yaml` for per-test fieldcompare configuration closes #837. | |
| - Add optional `tolerance` and `skip_compare` fields to `tests.yaml` for per-test fieldcompare configuration [#847](https://github.com/precice/tutorials/pull/847) |
|
|
||
|
|
||
| GLOBAL_TIMEOUT = int(os.environ.get("PRECICE_SYSTEMTESTS_TIMEOUT", 600)) | ||
| DEFAULT_FIELDCOMPARE_RTOL = 3e-7 |
There was a problem hiding this comment.
As more and more parameters accumulate here, I am wondering if we need a defaults dictionary, to reduce the number of arguments we pass aroung. But maybe implement that in a separate PR.
| def _log_skipped_fieldcompare(self) -> None: | ||
| log_sink = getattr(self, "_log_sink", None) | ||
| if log_sink is not None: | ||
| log_sink.begin_stage("compare") | ||
| log_sink.append_stdout( | ||
| f"(skipped: skip_compare=true, default rtol would be {DEFAULT_FIELDCOMPARE_RTOL})", | ||
| "compare", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Why is a dedicated function needed to log that the fieldcompare step was skipped?
| max_time_windows: int | None = None | ||
| timeout: int = GLOBAL_TIMEOUT | ||
| tolerance: float = DEFAULT_FIELDCOMPARE_RTOL | ||
| skip_compare: bool = False |
There was a problem hiding this comment.
I am a bit torn on whether the skip_comparison is needed. I would also rather formulate it positively: compare_results: false. One reason to remove it would be that it seems that most of the code additions come due to that. While we could completely skip the comparisons to work around failing runs, these would anyway fail at the run step.
I would suggest removing it. Setting a large tolerance already provides the capability to effectively skip comparisons.
2 participants
summary
closes #837
i added optional tolerance and skip_compare fields in tests.yaml so we can override fieldcompare rtol per test (or skip compare entirely if we only care about build + run).
before this we had some tests stuck in expected-to-fail or commented out in component suites because exported vtk values are tiny and the default 3e-7 rtol basically always fails. now those cases use tolerance: 1e-2 instead.
what changed
tests without tolerance still use the default 3e-7 so behavior for everything else should be the same.
local testing
i tried to run things locally on my machine , elastic-tube-3d (fluid-openfoam + solid-fenics) : full build / run / fieldcompare pass with -rtol 0.01 (32/32 comparisons) , this is the main case we cared about for this issue.
macro-nutils_micro-nutils : reference tarball doesn’t exist on develop yet so i couldn’t validate that one end-to-end
so yeah locally we’ve got solid proof for the fenics case ; the rest we’ll need ci .
notes for reviewers
would be great to trigger system tests on fenics-adapter, openfoam-adapter, micro-manager, dumux-adapter (and nutils-adapter once the nutils reference exists)
macro-nutils_micro-nutils is re-enabled in yaml but the reference file might still be missing upstream : heads up if ci complains there
happy to tweak tolerance values if 1e-2 is too loose or too tight for any of the three cases ,,